Skip to content

feat(ai): migrate localEmbeddingService onto WorkerBus v2 - #288

Merged
qnbs merged 13 commits into
mainfrom
feat/worker-v2-embedding-migration
Jul 30, 2026
Merged

feat(ai): migrate localEmbeddingService onto WorkerBus v2#288
qnbs merged 13 commits into
mainfrom
feat/worker-v2-embedding-migration

Conversation

@qnbs

@qnbs qnbs commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

Third of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). Stacked on #287 (DuckDB migration), which is stacked on #286 (parity gate). Plan: .claude/plans/worker-generation-v1-vs-enumerated-fox.md.

  • services/ai/localEmbeddingService.ts's public API (embedText/embedBatch/cosineSimilarity/clearEmbeddingCache) is unchanged — its consumers (localRagService.ts, crossProjectIndexService.ts, ragPromptAssembly.ts, ragVectorMigration.ts, proForgeMemoryBank.ts, loraEvaluationService.ts, loraDatasetBuilder.ts, inferenceGateway.ts) need no changes. Internals swapped from a dedicated new Worker(inference.worker.ts) instance to the shared WorkerBus v2 'inference' pool via ensureInferencePool() (same decoupling-from-enableWorkerBusV2 pattern as ensureWebLlmPool/ensureDuckDbPool).
  • v2's handler already throws on failure — matches embedText's existing throw-on-failure contract, so no response-shape translation adapter was needed here (unlike DuckDB's), just a transport swap.
  • Deleted ~70 lines of hand-rolled health-check/restart/backoff logic (30s ping, exponential backoff up to 5 attempts) — superseded by WorkerPool's generic health check.
  • Capped the shared 'inference' pool's maxWorkers to 2 (was MAX_WORKERS_INFERENCE=4) in workerBusManager.ts: each pool replica independently loads its own transformers.js pipeline with no cross-replica cache sharing, so 4 concurrent workers under a burst (e.g. embedBatch's micro-batching) could mean 4x the model memory footprint.

workers/inference.worker.ts (v1) is not deleted yet — localNlpService.ts still uses it directly; removal lands in the next PR of this sequence once that migration is done too.

Test plan

  • pnpm exec vitest run tests/unit/localEmbeddingService.test.ts — 18/18 (rewritten against a workerBusManager mock)
  • Full consumer suite: localRagService.test.ts, localRagService.duckdb.test.ts, ragPromptAssembly.test.ts, crossProjectIndexService.test.ts, crossProjectIndexDuckDb.test.ts, ragVectorMigration.test.ts, useConsistencyCheckerView.test.tsx, inferenceGateway.test.ts, proForgeMemoryBank.test.ts, loraEvaluationService.test.ts, loraDatasetBuilder.test.ts — 161/161 passing, unaffected since the public API didn't change
  • pnpm run typecheck (exact CI command) — clean
  • pnpm run lint (Biome) — clean via pre-commit hook
  • CI quality gate green (full coverage run — not run locally per this repo's low-end-hardware CI-cloud-first policy)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability when initializing inference concurrently.
    • Automatically re-registers the inference pool if it becomes unavailable.
    • Embedding failures now surface clearer errors when the inference service can’t be reached.
  • Performance

    • Embedding generation now routes through a shared WorkerBus v2 inference pool.
    • L2-normalization, caching, and batch embedding behavior remain supported.
  • Tests

    • Updated unit tests to mock WorkerBus v2 enqueueing and verify normalization, truncation, caching, and error propagation.
    • Added coverage for recovery and re-registration failure handling during inference initialization.

qnbs and others added 6 commits July 29, 2026 19:21
…ests

Neither workers/v2/duckdb.worker.ts's handlers nor
workers/v2/inference.worker.ts's handleInference were exported or
unit-tested against real logic (only webllm.worker.ts had that,
tests/unit/webllmWorkerHandler.test.ts) despite both already being
registered as live WorkerBus v2 pools with zero production callers —
docs/adr/0014-worker-generation-duplication.md's deferred migration
needs these to actually match v1 before any consumer cuts over.

Testing surfaced a real gap: initDuckDb()'s OPFS-unavailable catch
block silently swallowed the failure, unlike v1's out-of-band
OPFS_FALLBACK message that lets the UI warn users their analytics
won't persist. Fixed by threading ctx.emitProgress into initDuckDb()
and emitting an 'opfs-fallback' progress stage from the catch block —
reuses the existing progress channel WebLLM already uses for download
progress, no new message-protocol/schema change needed.

First PR of the ADR-0014 consolidation sprint (5 PRs total, see
.claude/plans/worker-generation-v1-vs-enumerated-fox.md).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…S-v3 format

CodeRabbit review on #286 found a real bug: initDuckDb()'s OPFS catch
block overwrote `connection` with the fallback connection without
closing the first (partially-attached) one, leaking a DuckDB
connection on every OPFS-attach failure. Fixed by tracking the OPFS
attempt in its own local variable and closing it in the catch path
before falling back. New regression test asserts close() is called.

Also collapsed several multi-line QNBS-v3 comments in
workers/v2/duckdb.worker.ts, workers/v2/inference.worker.ts, and both
new test files to the required single bracketed-line format.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Second of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication
(ADR-0014). services/duckdb/duckdbClient.ts's public API (init/query/
exec/shutdown/terminate/setOpfsFallbackHandler) is unchanged — its 5
real consumers (useDuckDb.ts, duckdbAnalytics.ts, duckdbMigration.ts,
ragVectorMigration.ts, telemetryService.ts) need no changes. Only the
internals swapped from a raw `new Worker(duckdbWorker.ts)` to routing
through the shared WorkerBus v2 'duckdb' pool via a new
ensureDuckDbPool() (mirrors ensureWebLlmPool()'s decoupling from the
enableWorkerBusV2 flag — analytics is core, not experimental infra).

The v2 worker returns raw values and throws on failure, unlike v1's
{ok,rows,error} wrapper — the adapter in duckdbClient.ts's send()
translates at that one boundary instead of touching ~20 call sites
across 5 files. WorkerBus's default 2-retry is disabled per-call
(retryPolicy: {maxRetries:0}) to avoid stacking under
duckdbAnalytics.ts's existing withDuckDbRetry() app-level retry.

Two things surfaced while wiring this up, both fixed here:
- WorkerBus.shutdown() tears down all 4 pools with no way to scope to
  just 'duckdb' — added WorkerBus.terminatePool(poolId) so
  duckdbClient.terminate() doesn't kill in-flight inference/webllm/
  plugin work on the shared bus.
- send() is async and awaits ensureDuckDbPool() before a task handle
  exists; naively attaching the AbortSignal listener after that await
  (like a first draft did) can lose a same-tick abort, unlike v1's
  synchronous Promise-executor version. Fixed by attaching the
  listener synchronously up front and applying it once the handle is
  ready — regression-tested.

workers/duckdbWorker.ts deleted; features/featureCatalog.ts's
enableDuckDbAnalytics entry retargeted to workers/v2/duckdb.worker.ts.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ailure

CodeRabbit's second pass on #286 flagged this as an outside-diff-range
comment (not a resolvable inline thread) on bcc41c2's own fix: the
new `await opfsConnection?.close()` call could itself reject, which
would skip emitProgress('opfs-fallback', ...) and the fallback
newDb.connect(), and replace the original ATTACH error with the close
failure — turning a graceful degrade into a hard init failure.

Wrapped the cleanup in its own try/catch (best-effort, logged via
console.warn, never blocks the fallback path) so a failing close()
can't prevent the fallback connection or swallow the real error. New
regression test asserts both still happen when close() rejects.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Third of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication
(ADR-0014). services/ai/localEmbeddingService.ts's public API
(embedText/embedBatch/cosineSimilarity/clearEmbeddingCache) is
unchanged — its consumers (localRagService.ts, crossProjectIndexService.ts,
ragPromptAssembly.ts, ragVectorMigration.ts, proForgeMemoryBank.ts,
loraEvaluationService.ts, loraDatasetBuilder.ts, inferenceGateway.ts)
need no changes. Internals swapped from a dedicated
`new Worker(inference.worker.ts)` instance to the shared WorkerBus v2
'inference' pool via ensureInferencePool() (same decoupling-from-
enableWorkerBusV2 pattern as ensureWebLlmPool/ensureDuckDbPool).

v2's handler already throws on failure (matching embedText's existing
throw-on-failure contract), so no response-shape translation is
needed here — just swap the transport. Deleted ~70 lines of hand-rolled
health-check/restart/backoff logic (30s ping, exponential backoff up
to 5 attempts), superseded by WorkerPool's generic health check.

Capped the shared 'inference' pool's maxWorkers to 2 (was
MAX_WORKERS_INFERENCE=4) in workerBusManager.ts: each pool replica
independently loads its own transformers.js pipeline with no
cross-replica cache sharing, so 4 concurrent workers under a burst
(e.g. embedBatch's micro-batching) could mean 4x the model memory
footprint.

workers/inference.worker.ts (v1) is NOT deleted yet — localNlpService.ts
still uses it directly; removal is PR 4 of this sequence, after that
migration lands too.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
worldscript-studio Ready Ready Preview Jul 30, 2026 2:00am

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 60bc7cd4-8b68-4feb-954b-dfa71e5e5793

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

Embedding generation now uses WorkerBus v2’s shared inference pool instead of a directly managed worker. Inference pool configuration and recovery are centralized, and tests cover task routing, errors, caching, batching, and pool re-registration.

Changes

WorkerBus inference flow

Layer / File(s) Summary
Inference pool configuration and recovery
services/workerBusManager.ts, tests/unit/workerBusManager.test.ts
Inference pool options are centralized and reused during initialization and re-registration. Missing-pool registration failures are logged while the live bus remains available, with tests covering DuckDB and inference recovery.
Embedding task routing and validation
services/ai/localEmbeddingService.ts, tests/unit/localEmbeddingService.test.ts
Embedding requests use ensureInferencePool() and enqueue inference.embed tasks; returned vectors are normalized while direct worker lifecycle code is removed. Tests cover unavailable pools, task errors, truncation, caching, and batch enqueue counts.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant embedText
  participant requestEmbedding
  participant ensureInferencePool
  participant WorkerBus
  participant InferenceWorker
  embedText->>requestEmbedding: submit embedding input
  requestEmbedding->>ensureInferencePool: acquire inference pool
  requestEmbedding->>WorkerBus: enqueue inference.embed
  WorkerBus->>InferenceWorker: execute task
  InferenceWorker-->>WorkerBus: return raw vector
  WorkerBus-->>requestEmbedding: return vector
  requestEmbedding-->>embedText: normalize vector
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the main change: migrating localEmbeddingService to WorkerBus v2.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/worker-v2-embedding-migration

Comment @coderabbitai help to get the list of available commands.

qnbs and others added 2 commits July 29, 2026 21:23
…QL param binding

Three real findings from CodeRabbit's review of #287 (duckdbClient
migration onto WorkerBus v2), all confirmed against current code
before fixing:

1. CRITICAL — params silently dropped (workers/v2/duckdb.worker.ts).
   duckdbClient.send() enqueues {sql, params}, but handleQuery/
   handleExec only read req.sql and called connection.query(sql)
   directly, ignoring params entirely and leaving an unbound-query/
   SQL-injection surface flagged by static analysis. This was already
   live: services/ai/telemetryService.ts's writeToDuckDb() passes real
   bound params ([taskType, backend, modelId, latencyMs, success])
   that were being silently discarded. Fixed via DuckDB-WASM prepared
   statements (connection.prepare(sql) + stmt.query(...params) +
   stmt.close()) when params is non-empty; unchanged direct
   connection.query(sql) otherwise. Needed a narrow structural type
   (PreparableConnection) since tsgo doesn't resolve
   AsyncDuckDBConnection.prepare() through this package's nested
   `export *` chain, despite it being present in the package's own
   .d.ts — same class of tsgo/external-package gap CLAUDE.md already
   documents for the transformers.js path alias.

2. MAJOR — DuckDB pool worker losing its connection on respawn
   (services/duckdb/duckdbClient.ts). The duckdb pool's minWorkers:0
   lets its worker idle-terminate; a freshly spawned replacement
   worker's module state has no `connection` until INIT runs again on
   it, so the next QUERY/EXEC after a respawn failed with "DuckDB not
   initialized". Fixed by transparently re-sending INIT once and
   retrying the original call when send() sees that specific error
   message (bounded to one retry, INIT/SHUTDOWN excluded from the
   check to avoid any recursive loop).

3. MAJOR — terminatePool() had no lifecycle contract
   (packages/worker-bus/src/workerBus.ts +
   services/workerBusManager.ts). Removing a pool didn't settle tasks
   already routed to it — their result promises would await a RESULT
   message from a worker that no longer exists. Fixed by tracking
   task->pool assignment (new taskPools map, set/cleared around
   runTask's pool-acquire phase) and cancelling matching tasks before
   deletion. Separately, ensureDuckDbPool() assumed a non-null bus
   always has every pool, so any call after duckdbClient.terminate()
   would fail with NO_POOL forever; added WorkerBus.hasPool() and a
   re-registration path (duckDbPoolOptions()/reRegisterDuckDbPool(),
   shared with doInitWorkerBus() to avoid option drift between the two
   registration sites).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…uilds

Root cause of both Vercel deployment failures on this branch/PR chain:
services/ai/localEmbeddingService.ts imports and calls
ensureInferencePool() from workerBusManager.ts, but that function was
never actually added there — only ensureDuckDbPool() was. Vitest never
caught this because localEmbeddingService.test.ts mocks the entire
workerBusManager module (vi.mock(...) replaces it wholesale), so the
real file's missing export was invisible to that suite. tsgo's
typecheck also passed clean locally for reasons still unclear (worth
a follow-up look), but rolldown's stricter static bundling in the
actual `pnpm run build:edge` (Vercel's exact build command) correctly
failed hard with [MISSING_EXPORT] — reproduced locally with
`NODE_OPTIONS=--max-old-space-size=3072 pnpm run build:edge`, which is
what actually surfaced this.

Added ensureInferencePool(), mirroring ensureDuckDbPool()'s shape
(decoupled from enableWorkerBusV2, re-registers the pool if it was
removed via terminatePool() while the bus stays alive). Refactored
doInitWorkerBus()'s inline 'inference' pool registration to share the
same inferencePoolOptions() the new function uses, avoiding the same
kind of drift the duckdb pool options already guard against.

New tests in workerBusManager.test.ts import the *real* module (only
@domain/worker-bus is mocked) — this exact suite would have caught the
missing export immediately, unlike the fully-mocked consumer-side
test. Verified fixed via a full local `pnpm run build:edge`
reproduction of Vercel's exact build command (previously failed with
[MISSING_EXPORT], now builds clean).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qnbs added a commit that referenced this pull request Jul 29, 2026
Root-caused the 2 Vercel build failures that prompted the mid-term
pause: not a platform issue, but a real missing-export bug
(services/ai/localEmbeddingService.ts imported ensureInferencePool()
from workerBusManager.ts, which never actually defined it — invisible
to Vitest because the consumer test mocks the whole module). The fix
landed in the worker-generation-consolidation PR chain (PR #288).

Reverts git.deploymentEnabled to the default (enabled) now that the
actual cause is fixed and verified via a clean local `build:edge`
reproduction of Vercel's exact build command. TODO.md's entry updated
from "paused, cause unknown" to a resolved write-up, including the
`pnpm run build` vs `pnpm run build:edge` command-mismatch lesson that
delayed finding this.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Base automatically changed from feat/worker-v2-duckdb-migration to main July 30, 2026 00:40
@codeant-ai

codeant-ai Bot commented Jul 30, 2026

Copy link
Copy Markdown

🏁 CodeAnt Quality Gate Results

Commit: b3e5d1af
Scan Time: 2026-07-30 02:00:56 UTC

✅ Overall Status: PASSED

Quality Gate Details

Quality Gate Status Details
Secrets ✅ PASSED 0 secrets found
Duplicate Code ✅ PASSED 0.0% duplicated
SAST ✅ PASSED No security issues
Bugs ✅ PASSED Rating S: No bugs
IAC ✅ PASSED No IAC issues

View Full Results

…ing-migration

# Conflicts:
#	packages/worker-bus/tests/workerBus.test.ts
#	services/duckdb/duckdbClient.ts
#	services/workerBusManager.ts
#	tests/unit/duckdbClient.test.ts
#	tests/unit/inferenceWorkerHandlerV2.test.ts
#	tests/unit/workerBusManager.test.ts
#	workers/v2/duckdb.worker.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@services/workerBusManager.ts`:
- Around line 86-91: Update ensureInferencePool() and ensureDuckDbPool() so
failures from reRegisterInferencePool() and the corresponding DuckDB
re-registration helper are caught after _bus is initialized. Log each
re-registration error and preserve the live bus instead of propagating the
rejection, while retaining null returns only for initial initialization
failures.

In `@tests/unit/workerBusManager.test.ts`:
- Around line 222-238: Add an assertion to the re-registration test around
ensureInferencePool and mockRegisterPool verifying the registration options
include maxWorkers: 2, so the test fails if the inference pool cap regresses to
the previous default.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5cc31091-4682-4854-aed5-005697a92001

📥 Commits

Reviewing files that changed from the base of the PR and between 021cdf9 and 73efc0d.

📒 Files selected for processing (4)
  • services/ai/localEmbeddingService.ts
  • services/workerBusManager.ts
  • tests/unit/localEmbeddingService.test.ts
  • tests/unit/workerBusManager.test.ts

Comment thread services/workerBusManager.ts
Comment thread tests/unit/workerBusManager.test.ts
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
services/workerBusManager.ts 88.88% 0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

ensureDuckDbPool() and ensureInferencePool() could reject after _bus was
already live because their re-registration helpers awaited a lazy import and
called registerPool() without a catch, breaking the documented "null only if
init failed" contract. Wrap both paths so a failed re-add logs instead of
propagating, and add a maxWorkers:2 assertion to the inference re-registration
test so the pool cap regression would be caught.

Addresses CodeRabbit review comments on PR #288.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qnbs

qnbs commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

The prior CodeRabbit fix (8a63b7c) added try/catch guards in
ensureDuckDbPool()/ensureInferencePool() so a pool re-registration failure
logs instead of propagating, but no test exercised the catch path — Codecov
flagged 4 missing lines (83.33% patch coverage). Adds one test per function
that forces registerPool() to throw and asserts the live bus is still
returned and the failure is logged via services/logger.ts.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
services/workerBusManager.ts (1)

87-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Apply the required adjacent QNBS-v3 rationale comments consistently.

  • services/workerBusManager.ts#L87-L90: add a one-line QNBS-v3 comment explaining missing-pool inference re-registration.
  • services/workerBusManager.ts#L125-L130: add a one-line QNBS-v3 comment explaining centralized inference-pool configuration.
  • tests/unit/workerBusManager.test.ts#L69-L79: add a one-line QNBS-v3 comment explaining logger mocking for recovery assertions.
  • tests/unit/workerBusManager.test.ts#L91-L91: add a one-line QNBS-v3 comment explaining mock reset isolation.
  • tests/unit/workerBusManager.test.ts#L268-L272: add a one-line QNBS-v3 comment explaining regression coverage for the inference worker cap.

As per coding guidelines, “Bei jeder inhaltlich relevanten Änderung in TypeScript- oder JavaScript-Dateien einen einzeiligen Kommentar im Format // QNBS-v3: [Grund / Impact / Kreativer Mehrwert] unmittelbar bei der Änderung ergänzen.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/workerBusManager.ts` around lines 87 - 90, Apply one-line comments
in the format “// QNBS-v3: [Grund / Impact / Kreativer Mehrwert]” immediately
beside each relevant change: in services/workerBusManager.ts lines 87-90,
explain missing inference-pool re-registration; at lines 125-130, explain
centralized inference-pool configuration; in tests/unit/workerBusManager.test.ts
lines 69-79, explain logger mocking for recovery assertions; at line 91, explain
mock-reset isolation; and at lines 268-272, explain regression coverage for the
inference worker cap.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@services/workerBusManager.ts`:
- Around line 87-90: Apply one-line comments in the format “// QNBS-v3: [Grund /
Impact / Kreativer Mehrwert]” immediately beside each relevant change: in
services/workerBusManager.ts lines 87-90, explain missing inference-pool
re-registration; at lines 125-130, explain centralized inference-pool
configuration; in tests/unit/workerBusManager.test.ts lines 69-79, explain
logger mocking for recovery assertions; at line 91, explain mock-reset
isolation; and at lines 268-272, explain regression coverage for the inference
worker cap.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8b0eace8-c875-471a-8942-b9ff6eeb63f9

📥 Commits

Reviewing files that changed from the base of the PR and between 73efc0d and a8686c4.

📒 Files selected for processing (2)
  • services/workerBusManager.ts
  • tests/unit/workerBusManager.test.ts

…de-diff-range review

Addresses CodeRabbit's 2 still-valid findings on PR #288: the logger mock
(why it's isolated from the real StructuredLogger) and the maxWorkers:2
regression assertion (why 2, not just what). The other 3 flagged spots
(reRegisterInferencePool, the inference registry.register block, and the
mockLogError.mockClear() line) mirror already-uncommented sibling code in
the same files (reRegisterDuckDbPool, the duckdb registry.register block,
6 other .mockClear() calls) — commenting only the new instance would be
less consistent, not more, so those are left as-is.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qnbs

qnbs commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Re: outside-diff-range QNBS-v3 comment finding (5 locations) — addressed in b3e5d1a.

Fixed (2): the logger mock and the maxWorkers: 2 regression assertion in tests/unit/workerBusManager.test.ts — both authored by me this session, both had genuinely non-obvious rationale worth documenting.

Not changed, with reasoning (3): reRegisterInferencePool (L87-90), the inference registry.register block (L125-130), and mockLogError.mockClear() (L91) each mirror an already-uncommented sibling in the same file (reRegisterDuckDbPool, the duckdb registry.register block, and 6 other .mockClear() calls respectively). Adding a comment only to the new instance would make the file less internally consistent, not more — and the "why" (memory-safety cap, pool re-registration after terminatePool(), mock-state isolation) is already documented once at the shared/canonical location (inferencePoolOptions()'s own comment, or the existing JSDoc).

@qnbs

qnbs commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@qnbs
qnbs merged commit 1e96606 into main Jul 30, 2026
26 checks passed
@qnbs
qnbs deleted the feat/worker-v2-embedding-migration branch July 30, 2026 02:22
@qnbs
qnbs restored the feat/worker-v2-embedding-migration branch July 30, 2026 02:26
@qnbs
qnbs deleted the feat/worker-v2-embedding-migration branch July 30, 2026 02:26
qnbs added a commit that referenced this pull request Jul 30, 2026
…ilures (#289)

* chore: pause Vercel auto-deploy mid-term after 2 undiagnosed build failures

Two consecutive Vercel preview deployments failed (different
deployment IDs, different commits) with only a generic "Deployment
has failed" message. The exact same commits build cleanly via
`pnpm run build` locally, and neither the GitHub Checks API nor
`npx vercel inspect --logs` (requires an interactive account login not
available here) surfaced any further detail — this looks
platform-side, not a code regression.

Vercel is not a required branch-protection check, so this was purely
about stopping a noisy, undiagnosable failure status rather than
unblocking anything. Disabled via vercel.json's documented
`git.deploymentEnabled: false` (the standard, reversible, dashboard-
free way to pause git-triggered deployments). GitHub Pages — already
documented as the always-on fallback mirror — is promoted to primary
in README.md/CLAUDE.md for the duration. TODO.md tracks re-enabling
once someone with Vercel dashboard/CLI access diagnoses the cause.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* revert: re-enable Vercel auto-deploy — root cause found and fixed

Root-caused the 2 Vercel build failures that prompted the mid-term
pause: not a platform issue, but a real missing-export bug
(services/ai/localEmbeddingService.ts imported ensureInferencePool()
from workerBusManager.ts, which never actually defined it — invisible
to Vitest because the consumer test mocks the whole module). The fix
landed in the worker-generation-consolidation PR chain (PR #288).

Reverts git.deploymentEnabled to the default (enabled) now that the
actual cause is fixed and verified via a clean local `build:edge`
reproduction of Vercel's exact build command. TODO.md's entry updated
from "paused, cause unknown" to a resolved write-up, including the
`pnpm run build` vs `pnpm run build:edge` command-mismatch lesson that
delayed finding this.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: correct README.md/CLAUDE.md revert (was reverted to own branch HEAD, not main)

The previous commit's `git checkout -- README.md CLAUDE.md` restored
from this branch's own HEAD (which still had the "paused" wording from
the first commit), not from main — so the pause note survived
uncommitted. Re-applied via `git checkout main -- ...` this time.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant